Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert codebase to Typescript #114

Merged
merged 32 commits into from
Feb 13, 2017
Merged

Convert codebase to Typescript #114

merged 32 commits into from
Feb 13, 2017

Conversation

alex3165
Copy link
Owner

@alex3165 alex3165 commented Feb 3, 2017

This PR include the following changes:

  • Convert all source files to typescript
  • Convert the test to typescript using ts-jest as a preprocessor

I would need help to review it, mainly the source files and have a look at the typescript interfaces.

PS: Examples are still written in ES6 compiled with Babel

@alex3165
Copy link
Owner Author

alex3165 commented Feb 6, 2017

@z0d14c @philpl @VilliGunn @ericrwolfe @arthur31416 Hey guys, as you are the most actives on the project I would like to ask you if you can have a look at this PR.

Sorry the PR is quite big

@z0d14c
Copy link
Contributor

z0d14c commented Feb 7, 2017

Hi, I didn't take a fine-toothed comb to the PR but it looks good from a bird's-eye view.

edit: As a further detail: I am not a typescript expert. That being said, I thoroughly support introducing typing to the project. The one caveat I would add is that if the addition of TS significantly changes development (probably yes) or installation (hopefully not) of the project, can we please document that in the README. :)

tsconfig.json Outdated
"compilerOptions": {
"outDir": "lib",
"module": "commonjs",
"target": "es6",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing babel and setting ts target to es6 means that you don't support non es6 compatible browser.

You may want to set this to es5 but be aware that ts doesn't transpile es6 to es5 out of the box (e.g microsoft/TypeScript#11209).
In case of errors, you may need to reintroduce babel after tsc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Wykks Yep, totally correct, except TS does compile ES6 to ES5, except for very specific staged things like: Iterables / Iterators, Async / Await & Generators

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I meant :)
(And it will be fixed soon microsoft/TypeScript#12346)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the target to ES5, here is the commit: ec97fb7

@calocan
Copy link

calocan commented Feb 7, 2017

The pull request doesn't seem to build, something isn't parsing in feature.ts for me

 npm run lint && npm run test && tsc


> [email protected] lint /Users/andy/code/react-mapbox-gl
> tslint --project tsconfig.json


> [email protected] test /Users/andy/code/react-mapbox-gl
> jest

 PASS  src/__tests__/layer.test.tsx
  Layer
    ✓ Should render layer with default options (16ms)
    ✓ Should render layer with default source (2ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        1.397s
Ran all test suites.
src/feature.ts(11,7): error TS2322: Type '() => null' is not assignable to type 'StatelessComponent<Props>'.
  Type 'null' is not assignable to type 'ReactElement<any>'.

npm ERR! Darwin 15.6.0
npm ERR! argv "/Users/andy/.nvm/versions/node/v7.3.0/bin/node" "/Users/andy/.nvm/versions/node/v7.3.0/bin/npm" "run" "build"
npm ERR! node v7.3.0
npm ERR! npm  v4.1.2
npm ERR! code ELIFECYCLE
npm ERR! [email protected] build: `npm run lint && npm run test && tsc`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the [email protected] build script 'npm run lint && npm run test && tsc'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the react-mapbox-gl package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm run lint && npm run test && tsc
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs react-mapbox-gl
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls react-mapbox-gl
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/andy/code/react-mapbox-gl/npm-debug.log

npm WARN [email protected] requires a peer of jest@^18.0.0 but none was installed.
npm ERR! Darwin 15.6.0
npm ERR! argv "/Users/andy/.nvm/versions/node/v7.3.0/bin/node" "/Users/andy/.nvm/versions/node/v7.3.0/bin/npm" "install"
npm ERR! node v7.3.0
npm ERR! npm  v4.1.2
npm ERR! code ELIFECYCLE
npm ERR! [email protected] prepublish: `npm run clean && npm run build`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] prepublish script 'npm run clean && npm run build'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the react-mapbox-gl package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm run clean && npm run build
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs react-mapbox-gl
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls react-mapbox-gl
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/andy/code/react-mapbox-gl/npm-debug.log

Copy link
Contributor

@Wykks Wykks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just read cluster.tsx for now, hope this help :)

package.json Outdated
"@types/react": "^15.0.6",
"@types/react-addons-test-utils": "^0.14.17",
"@types/recompose": "^0.20.3",
"@types/typescript": "^2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use typescript instead of @types/typescript

src/cluster.tsx Outdated
const supercluster = require('supercluster'); //tslint:disable-line

export interface Props {
ClusterMarkerFactory: (coordinates: number[], pointCount: number) => JSX.Element;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can write this like this :
ClusterMarkerFactory(coordinates: number[], pointCount: number): JSX.Element;
(tslint rule : https://palantir.github.io/tslint/rules/prefer-method-signature/)

What do you think about it?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

extent?: number;
nodeSize?: number;
log?: boolean;
children?: Array<React.Component<MarkerProps, void>>;
Copy link
Contributor

@Wykks Wykks Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use either Array<T> or T[]
You should set https://palantir.github.io/tslint/rules/array-type/

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using array-simple already but React.Component<MarkerProps, void> is not a simple type

Copy link
Contributor

@Wykks Wykks Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay make sense, missed array-simple.
Although, I'm not sure if mixing two different syntax for the same purpose is a good idea.

@@ -0,0 +1,136 @@
import * as React from 'react';
import * as MapboxGL from 'mapbox-gl';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing MapboxGL this way, add a hard dependency to Mapboxgl in this file, while it's not necessary.
You just need the type, you can use the ambient definition mapboxgl instead.
Or you don't want to use ambient definition ? (which is understandable)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if by using the ambient definition you mean this

/// <renference type="..."/>

I prefer not to use it because if i understand it well it expose globaly all the types of your reference in your project using this right ? even though I think it is difficult to track where the type come from

Copy link
Contributor

@Wykks Wykks Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly, with @ types, every types inside the @ type directory is already loaded. So you can already use mapboxgl as is.
But sometimes, namespace are not exposed globally and you have to use the import for type definition...
Also typescript does elide this kind of import but I'm not sure exactly when (microsoft/TypeScript#2812)
Since I'm not sure about how typescript transpile this, I personnally perfer to not use import when it's not necessary.

src/cluster.tsx Outdated
import * as MapboxGL from 'mapbox-gl';
import { Props as MarkerProps } from './marker';

const supercluster = require('supercluster'); //tslint:disable-line
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here require is used to get around noImplicitAny(and tslint).
I think it's better to write something like this (in a global.d.ts file):

declare module 'supercluster' {
    const supercluster: any;
    export default supercluster;
}

(Repeat this comment for each require like this)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 2ea4e37

"eslint-plugin-react": "^6.5.0",
"@types/jest": "^18.1.1",
"@types/mapbox-gl": "^0.29.0",
"@types/node": "^7.0.5",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove every require you don't need this anymore (+ it's a bit overkill to add node types definition just for require)

src/cluster.tsx Outdated
}
}

private feature(coordinates: number[]) {
Copy link
Contributor

@Wykks Wykks Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private feature(coordinates: GeoJSON.Position): GeoJSON.Feature<GeoJSON.Point>

(You can add import * as GeoJSON from 'geojson' instead of using ambient type def)

GeoJSON is added by @types/mapbox-gl but you should explicitly add it too since you're using it

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since GeoJSON.Feature is not right I have done this: private feature(coordinates: GeoJSON.Position): Point

src/cluster.tsx Outdated
map: MapboxGL.Map;
}

export interface Point {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, use GeoJSON.Feature<GeoJSON.Point> instead

Copy link
Owner Author

@alex3165 alex3165 Feb 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type definition for GeoJSON.Feature is fucked : properties: {} | null; and should be properties?: {[key: string]: any}

Copy link
Contributor

@Wykks Wykks Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the issue exactly ?
I don't think that properties should be optional (http://geojson.org/geojson-spec.html#feature-objects)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is not optional it is {} which mean empty object so you cannot pass any property

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, that's quite new.
You can use the previous version of @types/geojson then
"@types/geojson": "0.0.32"

src/cluster.tsx Outdated
return (
<div>
{// tslint:disable-line
clusterPoints.map(({ geometry, properties }: Point) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clusterPoints.map(({ geometry, properties }: GeoJSON.Feature<GeoJSON.Point>)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same GeoJSON.Feature is not right

@alex3165
Copy link
Owner Author

alex3165 commented Feb 8, 2017

@calocan Thanks for the catch, it should compile correctly now

@@ -0,0 +1,138 @@
import * as React from 'react';
import * as MapboxGL from 'mapbox-gl';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thing will totally destroy tree-shaking. Same goes for * as React. Don't do that in libraries ;)

extent?: number;
nodeSize?: number;
log?: boolean;
children?: Array<React.Component<MarkerProps, void>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency, I have to agree here. It's Array<> here and sometimes x[] below

}

export interface State {
superC: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of anys... Is that a good thing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, supercluster should be (manually) typed since it cause too much any.

map: React.PropTypes.object
};

public static defaultProps = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure that you don't need to say that a static variable is public, since private/protected static shouldn't be a thing

const { children } = this.props;

if (children) {
const features = this.childrenToFeatures(children as any);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As any?

src/marker.tsx Outdated
public render() {
return (
<ProjectedLayer
{...{ ...this.props, children: undefined}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't nice to read... Also it's unnecessary. 1) It might not compile to just createElement(ProjectedLayer, this.props), but if it does, that'd be fine, because immutability is guaranteed on these objects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this could be a SFC

public render() {
return (
<ProjectedLayer
{...{ ...this.props, children: undefined }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

map: PropTypes.object,
};
export interface Props {
coordinates: number[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same: number[][]

Actually... Since it's always a tuple: [number, number][]

const { measurement } = this.props;

const clientWidth = map._canvas.clientWidth;
const { _ne, _sw } = map.getBounds();
const clientWidth = (map as any)._canvas.clientWidth;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use of any

src/source.ts Outdated
(map
.getSource(id) as GeoJSONSource)
.setData((props.sourceOptions as GeoJSONSourceRaw).data as any);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh god

Copy link
Contributor

@Wykks Wykks Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit tricky here.
@alex3165
First, simplify sourceOptions type. You don't really need this :

export type Sources = VectorSource | RasterSource | GeoJSONSource | ImageSource | VideoSource | GeoJSONSourceRaw;

Just do this :
sourceOptions: GeoJSONSourceRaw;

And then you can write this line like this :

if (props.sourceOptions.data !== sourceOptions.data) {
      const source = map.getSource(id) as GeoJSONSource;
      const data = props.sourceOptions.data as GeoJSON.Feature<GeoJSON.GeometryObject> | GeoJSON.FeatureCollection<GeoJSON.GeometryObject> | string;
      source.setData(data);
}

By the way the long type GeoJSON.Feature<GeoJSON.GeometryObject> | GeoJSON.FeatureCollection<GeoJSON.GeometryObject> | string is also present in geojson-layer.ts, so you can make a type alias of this.

@@ -1,3 +1,4 @@
// tslint:disable
Copy link
Contributor

@Wykks Wykks Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to disable the concerned rule instead of disabling everything:
// tslint:disable:max-line-length

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 61cd6d6

tsconfig.json Outdated
"noImplicitThis": true,
"noImplicitAny": true,
"strictNullChecks": true,
"suppressImplicitAnyIndexErrors": true,
Copy link
Contributor

@Wykks Wykks Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kill it with 🔥
Seriously this allow stuff like :

type obj = { stuff: string };
const foo: obj = {stuff: ''};
const data = 'notstuff';
const bar = foo[data];

This option is old, now we can use keyof for better typing:

type obj = { stuff: string };
const foo: obj = {stuff: ''};
const data: keyof obj = 'notstuff'; // Type '"notstuff"' is not assignable to type '"stuff"'
const bar = foo[data];

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 9bb5f83

@alex3165 alex3165 merged commit 768679b into master Feb 13, 2017
@alex3165 alex3165 deleted the typescript branch February 23, 2017 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants